workers: experimental BroadcastChannel #36271

jasnell · 2020-11-25T22:56:28Z

Currently very experimental and definitely not finished implementation of BroadcastChannel for Node.js.

The intent for this is for it to be completely compatible with the browser API. It's not there yet.

For folks who are not familiar with BroadcastChannel, it is essentially a one-to-many alternative to MessageChannel.

'use strict';

const {
  isMainThread,
  BroadcastChannel,
  Worker
} = require('worker_threads');

const bc = new BroadcastChannel('hello');

if (isMainThread) {
  let c = 0;
  bc.onmessage = (event) => {
    console.log(event.data);
    if (++c === 10) bc.close();
  };
  for (let n = 0; n < 10; n++)
    new Worker(__filename);
} else {
  bc.postMessage('hello from every worker');
  bc.close();
}

All BroadcastChannel instances that share the same channel name, created in the main thread or worker threads, will receive a copy of the posted message.

Message delivery is at-most once, with no retention. When a message is posted, it is synchronously copied and queued into each of the other instances. Those other instances will deliver the message copy asynchronously.

Transferables are not supported with BroadcastChannel, so only messages that can be cloned are supported.

One use case of BroadcastChannel is to signal a group of worker threads that a process is shutting down, without having to send individual signals to each worker:

Worker:

const bc = new BroadcastChannel('pool-controller');
bc.onmessage = (({data}) => {
  if (data.shutdown)
    console.log('Shutting down!');
  bc.postMessage({done:true});
};

// Do worker stuff...

Main:

const bc = new BroadcastChannel('pool-controller');
bc.onmessage = ({data}) => console.log(data.done);
new Worker('worker.js');
setTimeout(() => bc.postMessage({shutdown:true}), 1000);

@addaleax: I'd be interested in your opinions on how to improve the C++ part of the implementation.
@benjamingr: I'd be interested in your thoughts on the JavaScript side of the implementation.

This is definitely a work in progress still, motivated by some needs around Piscina.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

addaleax

I’ll have to think a bit about how this works on the C++ side … this definitely breaks the MessagePort/MessagePortData encapsulation in a big way, and probably in a way that can be avoided, and probably with less code than this would currently introduce…

src/node_messaging.cc

src/node_messaging.h

src/node_messaging.cc

addaleax · 2020-11-26T10:15:17Z

Basically, what I think I’d rather aim for is:

Making Messages deserializable multiple times, rather than just once
Instead of allowing only 1:1 sibling relationships on MessagePortData instances:
- Introduce a SiblingGroup class (or similar name) that would extend to allowing more than 2 MessagePortData instances to be entangled with each other
- Keeping a global [channel name] →SiblingGroup map so
Keep using regular MessagePorts instead of custom BroadcastPorts

addaleax · 2020-11-26T18:16:18Z

@jasnell Fwiw, if you want, I can probably find the time to write a prototype for that suggestion, and you can see if you like it or not :)

jasnell · 2020-11-26T18:22:56Z

Thanks :-) I've already started working it up (yes it's a holiday here but this is Fun Coding not Work Coding so it's all good :-)...lol). The SiblingGroup is pretty straightforward. The one question I would have is what we should do if a Message that has transferables is dispatched to a SiblingGroup with more than one destination MessagePortData.

addaleax · 2020-11-26T18:28:35Z

The one question I would have is what we should do if a Message that has transferables is dispatched to a SiblingGroup with more than one destination MessagePortData.

Yeah, I think there’s not much we can do besides fail … but we can do that during sending, right?

jasnell · 2020-11-26T18:33:11Z

Yeah, I think there’s not much we can do besides fail … but we can do that during sending, right?

I think so. We can fail in PostMessage if the SiblingGroup has more than one destination port.

jasnell · 2020-11-26T22:47:10Z

@addaleax ... ok, completely revised the implementation as discussed. Please take a look. Here, I'm done for the day and ready to go eat some turkey.

addaleax

Yeah, I really like where this is going :)

src/node_messaging.cc

src/node_messaging.h

jasnell · 2020-11-30T19:20:55Z

@addaleax @benjamingr ... marking this ready for review now. There's still work that can be done on this to ensure that it's completely up to spec.

nodejs-github-bot · 2020-11-30T19:29:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34637/

addaleax

Looks good, apart from the error code thing :)

src/node_errors.h

src/node_messaging.cc

jasnell · 2020-11-30T23:07:59Z

@addaleax .. updated! :-)

addaleax

Nice :)

src/node_messaging.cc

nodejs-github-bot · 2020-11-30T23:20:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34638/

nodejs-github-bot · 2020-11-30T23:31:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34639/

Signed-off-by: James M Snell <jasnell@gmail.com>

benjamingr · 2020-12-01T12:11:12Z

doc/api/worker_threads.md

+
+Closes the `BroadcastChannel` connection.
+
+### `broadcastChannel.onmessage`


Any reason these are documented as event handlers and not event "message" etc?

Because from the documentation I've been able to find on web usage, the onmessage pattern tends to be more common. We can tweak the documentation later if necessary.

Just to clarify because of the "we can tweak the docs later" comment - a LGTM from me with comments always means "This is fine to land but I have these comments/questions we should probably address at some point".

If I think something isn't ready to land I never LGTM, I typically (like in this case) check the code out, play with it, put a debugger (in test/parallel/test-worker-broadcastchannel.js here, kind of annoying in worker_threads because I do it from ndb and not vscode), add some logs and LGTM.

I typically don't leave style nits because I usually really don't care about those.

There are a few (very minor) nits here that I might follow up with a PR about later if someone ever complains about them namely stuff like onmessage not being logged when you util.inspect a BroadcastChannel here but it being logged in Chrome. I honestly think these things aren't worth mentioning in PRs most of the time.

benjamingr

JS LGTM

The API itself leaves a lot to be desired but I guess we should follow the DOM's MessagePort so little we can do about it here :]

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #36271 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

jasnell · 2020-12-01T16:22:49Z

Landed in 9e446b3

This addresses the `TODO` left on my request in 9e446b3. :) Refs: nodejs#36271

Trott · 2020-12-02T13:41:56Z

This looks like it landed without a CI run after the rebase. Likely related: Windows CI now fails with the test added here.

Trott · 2020-12-02T14:04:01Z

This looks like it landed without a CI run after the rebase. Likely related: Windows CI now fails with the test added here.

I don't have a Windows machine to test on locally, but I've optimistically opened #36353 changing the expected Windows behavior to be the spec-compliant behavior mentioned in the comment. If that passes CI, then I'll propose fast-tracking.

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #36271 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #36435 Notable changes: * child_processes: * add AbortSignal support (Benjamin Gruenbaum) (#36308) * deps: * update ICU to 68.1 (Michaël Zasso) (#36187) * events: * support signal in EventTarget (Benjamin Gruenbaum) (#36258) * graduate Event, EventTarget, AbortController (James M Snell) (#35949) * http: * enable call chaining with setHeader() (pooja d.p) (#35924) * module: * add isPreloading indicator (James M Snell) (#36263) * stream: * support abort signal (Benjamin Gruenbaum) (#36061) * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922) * worker: * add experimental BroadcastChannel (James M Snell) (#36271)

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: nodejs#36271 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #36435 Notable changes: * child_processes: * add AbortSignal support (Benjamin Gruenbaum) (#36308) * deps: * update ICU to 68.1 (Michaël Zasso) (#36187) * events: * support signal in EventTarget (Benjamin Gruenbaum) (#36258) * graduate Event, EventTarget, AbortController (James M Snell) (#35949) * http: * enable call chaining with setHeader() (pooja d.p) (#35924) * module: * add isPreloading indicator (James M Snell) (#36263) * stream: * support abort signal (Benjamin Gruenbaum) (#36061) * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922) * worker: * add experimental BroadcastChannel (James M Snell) (#36271)

This addresses the `TODO` left on my request in 9e446b3. :) Refs: #36271 PR-URL: #36345 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos · 2021-08-04T11:57:03Z

This will need a backport to land on v14.x

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 25, 2020

jasnell requested review from addaleax and benjamingr November 25, 2020 22:56

jasnell added semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support. experimental Issues and PRs related to experimental features. labels Nov 25, 2020

addaleax reviewed Nov 26, 2020

View reviewed changes

src/node_messaging.cc Outdated Show resolved Hide resolved

src/node_messaging.cc Outdated Show resolved Hide resolved

src/node_messaging.h Outdated Show resolved Hide resolved

src/node_messaging.cc Outdated Show resolved Hide resolved

jasnell force-pushed the broadcastchannel branch 2 times, most recently from 8a74b55 to 08ab8d6 Compare November 26, 2020 22:45

jasnell mentioned this pull request Nov 26, 2020

ParentPort with multiple workers piscinajs/piscina#98

Closed

addaleax reviewed Nov 26, 2020

View reviewed changes

src/node_messaging.cc Outdated Show resolved Hide resolved

src/node_messaging.cc Outdated Show resolved Hide resolved

src/node_messaging.cc Show resolved Hide resolved

src/node_messaging.cc Outdated Show resolved Hide resolved

src/node_messaging.h Outdated Show resolved Hide resolved

jasnell force-pushed the broadcastchannel branch from 08ab8d6 to e31c009 Compare November 30, 2020 19:18

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020

jasnell marked this pull request as ready for review November 30, 2020 19:20

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020

addaleax requested changes Nov 30, 2020

View reviewed changes

src/node_errors.h Outdated Show resolved Hide resolved

src/node_messaging.cc Outdated Show resolved Hide resolved

src/node_messaging.cc Show resolved Hide resolved

src/node_messaging.cc Outdated Show resolved Hide resolved

src/node_messaging.cc Outdated Show resolved Hide resolved

jasnell force-pushed the broadcastchannel branch from e31c009 to d1c68a2 Compare November 30, 2020 23:07

jasnell requested a review from addaleax November 30, 2020 23:07

addaleax approved these changes Nov 30, 2020

View reviewed changes

src/node_messaging.cc Outdated Show resolved Hide resolved

jasnell force-pushed the broadcastchannel branch from d1c68a2 to 5e06220 Compare November 30, 2020 23:17

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020

jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 30, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020

workers: add experimental BroadcastChannel

6066197

Signed-off-by: James M Snell <jasnell@gmail.com>

jasnell force-pushed the broadcastchannel branch from 94d2d6f to 6066197 Compare December 1, 2020 01:06

jasnell mentioned this pull request Dec 1, 2020

Api allows workers to do initialization but it is not clear how to do cleanup piscinajs/piscina#87

Closed

benjamingr reviewed Dec 1, 2020

View reviewed changes

benjamingr approved these changes Dec 1, 2020

View reviewed changes

jasnell added a commit that referenced this pull request Dec 1, 2020

worker: add experimental BroadcastChannel

9e446b3

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #36271 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

jasnell closed this Dec 1, 2020

addaleax added a commit to addaleax/node that referenced this pull request Dec 1, 2020

worker: refactor MessagePort entanglement management

6334542

This addresses the `TODO` left on my request in 9e446b3. :) Refs: nodejs#36271

addaleax mentioned this pull request Dec 1, 2020

worker: refactor MessagePort entanglement management #36345

Closed

2 tasks

danielleadams pushed a commit that referenced this pull request Dec 7, 2020

worker: add experimental BroadcastChannel

802d44b

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #36271 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

danielleadams mentioned this pull request Dec 7, 2020

v15.4.0 proposal #36435

Merged

targos added the backport-requested-v14.x label Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workers: experimental BroadcastChannel #36271

workers: experimental BroadcastChannel #36271

jasnell commented Nov 25, 2020 •

edited

addaleax left a comment

addaleax commented Nov 26, 2020

addaleax commented Nov 26, 2020 •

edited

jasnell commented Nov 26, 2020

addaleax commented Nov 26, 2020

jasnell commented Nov 26, 2020

jasnell commented Nov 26, 2020

addaleax left a comment

jasnell commented Nov 30, 2020

nodejs-github-bot commented Nov 30, 2020

addaleax left a comment

jasnell commented Nov 30, 2020

addaleax left a comment

nodejs-github-bot commented Nov 30, 2020

nodejs-github-bot commented Nov 30, 2020

benjamingr Dec 1, 2020

jasnell Dec 1, 2020 •

edited

benjamingr Dec 1, 2020

benjamingr left a comment

jasnell commented Dec 1, 2020

Trott commented Dec 2, 2020 •

edited

Trott commented Dec 2, 2020

targos commented Aug 4, 2021


		Closes the `BroadcastChannel` connection.

		### `broadcastChannel.onmessage`

workers: experimental BroadcastChannel #36271

workers: experimental BroadcastChannel #36271

Conversation

jasnell commented Nov 25, 2020 • edited

Checklist

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented Nov 26, 2020

addaleax commented Nov 26, 2020 • edited

jasnell commented Nov 26, 2020

addaleax commented Nov 26, 2020

jasnell commented Nov 26, 2020

jasnell commented Nov 26, 2020

addaleax left a comment

Choose a reason for hiding this comment

jasnell commented Nov 30, 2020

nodejs-github-bot commented Nov 30, 2020

addaleax left a comment

Choose a reason for hiding this comment

jasnell commented Nov 30, 2020

addaleax left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 30, 2020

nodejs-github-bot commented Nov 30, 2020

benjamingr Dec 1, 2020

Choose a reason for hiding this comment

jasnell Dec 1, 2020 • edited

Choose a reason for hiding this comment

benjamingr Dec 1, 2020

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

jasnell commented Dec 1, 2020

Trott commented Dec 2, 2020 • edited

Trott commented Dec 2, 2020

targos commented Aug 4, 2021

jasnell commented Nov 25, 2020 •

edited

addaleax commented Nov 26, 2020 •

edited

jasnell Dec 1, 2020 •

edited

Trott commented Dec 2, 2020 •

edited